Skip to content

Send GeoJSON data from worker to main if loaded from a URL #6811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Send GeoJSON data from worker to main if loaded from a URL #6811

wants to merge 5 commits into from
+30 −4

Conversation

@lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Dec 4, 2025 •
edited
Loading

Summary

Part of #4364

This PR carves out an exception from the optimization added in #6668. When GeoJSON data is loaded from a URL, we now send it back to the main thread so that it can be:

When GeoJSON is provided inline or via diff updates, the main thread already has the data, so we skip sending it back (preserving the #6668 optimization).

This will make GeoJSONSource#updateData much faster when the GeoJSON data was loaded from a URL.

Benchmarks

$ npm run benchmark -- GeoJSONSourceUpdateData && npm run benchmark -- GeoJSONSourceSetData 

> [email protected] benchmark
> node --no-warnings --loader ts-node/esm test/bench/run-benchmarks.ts GeoJSONSourceUpdateData

Starting headless chrome at: http://localhost:9966/test/bench/versions/index.html
                                      v5.14.0                  main  set-data-url 85c1f56 
 GeoJSONSourceUpdateData                                92.3814 ms            92.6344 ms 

> [email protected] benchmark
> node --no-warnings --loader ts-node/esm test/bench/run-benchmarks.ts GeoJSONSourceSetData

Starting headless chrome at: http://localhost:9966/test/bench/versions/index.html
                                   v5.14.0                  main  set-data-url 85c1f56 
 GeoJSONSourceSetData                               279.7234 ms           284.3850 ms

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license)
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@lucaswoj lucaswoj changed the title Send GeoJSON data from worker to main thread IF loaded from a URL Send GeoJSON data from worker to main if loaded from a URL Dec 4, 2025
@lucaswoj lucaswoj mentioned this pull request Dec 4, 2025
Draft
8 tasks
@codecov
Copy link

codecov bot commented Dec 4, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.42%. Comparing base (f681bbb) to head (6af1a9b).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #6811   +/-   ##
=======================================
  Coverage   92.42%   92.42%           
=======================================
  Files         288      288           
  Lines       23807    23809    +2     
  Branches     5056     5058    +2     
=======================================
+ Hits        22003    22005    +2     
  Misses       1804     1804

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@HarelM
Copy link
Collaborator

HarelM commented Dec 5, 2025

How about fetching it in the main thread instead of serializing it from the worker?
I think the browser will cache the response so the worker won't really fetch it again.
This is similar to what we did in the rtl plugin fetching.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Dec 5, 2025

There's no way to guarantee the browser will cache the response. If the response has a Cache-Control: no-store header, for example, it will certainly be fetched twice.

HarelM
HarelM reviewed Dec 5, 2025
View reviewed changes
HarelM
HarelM reviewed Dec 5, 2025
View reviewed changes
@HarelM
Copy link
Collaborator

HarelM commented Dec 5, 2025

Does it make sense now to return the data in getData method not from the worker but from the main thread class?

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Dec 5, 2025

Does it make sense now to return the data in getData method not from the worker but from the main thread class?

As we discussed in #6738, there are still cases where GeoJSONSource#getData returns a different value than GeoJSONSource#serialize

... if you called setData with some improperly wound GeoJSON geometries, and then called Map#getStyle, you'd get the rewound geometries back. Even though this is undocumented behavior, I suspect at least a couple folks are relying on it. They will need to use the GeoJSONSource#getData method instead.

I think it's worth having this method around for backwards compatibility.

(Sidenote: If and when we're ready to make some breaking changes, I'd love to see GeoJSONSource#serialize and Map#getStyle become async and pull this giant dataset from the worker only when it's requested)

@lucaswoj lucaswoj requested a review from HarelM December 5, 2025 07:44
@HarelM
Copy link
Collaborator

HarelM commented Dec 5, 2025

If you have ideas for breaking changes please add them to the following issue:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HarelM HarelM Awaiting requested review from HarelM

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lucaswoj @HarelM